Skip to content

Add check for deadcode for CI#2631

Merged
robertbrignull merged 7 commits intomainfrom
robertbrignull/deadcode_ci
Jul 25, 2023
Merged

Add check for deadcode for CI#2631
robertbrignull merged 7 commits intomainfrom
robertbrignull/deadcode_ci

Conversation

@robertbrignull
Copy link
Copy Markdown
Contributor

@robertbrignull robertbrignull commented Jul 24, 2023

This PR installs ts-unused-exports and uses it to find dead code. This PR doesn't fix any existing violations, so the CI will fail until we fix those in #2632.

The specific thing that this tools helps find is code that is exported but then not imported from anywhere else. This either means the code can be unexported if it's still used within its file, or it can be deleted.

By virtue of typescript-eslint/no-unused-vars we already have detection of unexported code (including functions/types/variables) that is not used elsewhere within its file. Using ts-unused-exports allows us to go one step further and look for unnecessary exports, and then once unexported our existing eslint rules will find any unused code.

Checklist

  • CHANGELOG.md has been updated to incorporate all user visible changes made by this pull request.
  • Issues have been created for any UI or other user-facing changes made by this pull request.
  • [Maintainers only] If this pull request makes user-facing changes that require documentation changes, open a corresponding docs pull request in the github/codeql repo and add the ready-for-doc-review label there.

@robertbrignull robertbrignull requested a review from a team July 24, 2023 10:33
@robertbrignull robertbrignull requested a review from a team as a code owner July 24, 2023 10:33
@@ -0,0 +1,5 @@
{
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One problem I had writing this was that our codebase is split into multiple different projects with multiple tsconfig.json files that all depend on each other, and all the tools only operate on a single project at a time. Unfortunately just looking at one project you can't tell if a piece of code is unused or not, because it could be used from another project that depends on this one. To work around this I created a new find-deadcode-tsconfig.json file that covers the entire repository. I'm not totally sure this means the dead code detection is 100% accurate, and there could be edge cases since the different projects use different rules, but I think it's likely to be good enough.
(Side note: it can't be named tsconfig.json because that file already exists in that directory. I could put it in another directory, but I'm not sure which one.)

@robertbrignull
Copy link
Copy Markdown
Contributor Author

I did try out other tools but I found that ts-unused-exports worked the best.

  • import/no-unused-modules just wasn't working for me. It wasn't detecting any unused exports, and I know that there are lots in the repo but the other methods did detect them. I'm not sure what's going on with this.
  • ts-prune worked better, but sadly it appears it doesn't understand .tsx files, maybe. It was declaring lots of react components to be unused when they clearly were used from other components.

Comment thread extensions/ql-vscode/package.json Outdated
"format": "prettier --write **/*.{ts,tsx} && eslint . --ext .ts,.tsx --fix",
"lint": "eslint . --ext .js,.ts,.tsx --max-warnings=0",
"lint:markdown": "markdownlint-cli2 \"../../**/*.{md,mdx}\" \"!**/node_modules/**\" \"!**/.vscode-test/**\" \"!**/build/cli/v*/**\"",
"find-deadcode": "ts-unused-exports find-deadcode-tsconfig.json --showLineNumber --excludePathsFromReport=$(find src test -type f '(' -name jest.config.ts -or -name 'index.tsx' -or -name 'index.ts' ')' -print0 | tr '\\0' ';')'gulpfile.ts/;src/stories/;test/vscode-tests/jest-runner-installed-extensions.ts'",
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are quite a few files that either have to have unused exported items by design (e.g. the index.tsx files or anything in src/stories/), and perhaps that we just don't care as much about so it's easier to ignore them to avoid jumping through hoops. Feel free to say if you think this list is not correct.

Copy link
Copy Markdown
Member

@koesie10 koesie10 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding this!

Comment thread extensions/ql-vscode/package.json Outdated
Comment thread extensions/ql-vscode/find-deadcode-tsconfig.json
Copy link
Copy Markdown
Member

@koesie10 koesie10 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for converting it to a script!

@robertbrignull robertbrignull enabled auto-merge July 25, 2023 11:48
@robertbrignull robertbrignull merged commit e1bbbd6 into main Jul 25, 2023
@robertbrignull robertbrignull deleted the robertbrignull/deadcode_ci branch July 25, 2023 12:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants